-
Notifications
You must be signed in to change notification settings - Fork 26.6k
Fix: Stabilize ExtensionLoaderTest.testInjectExtension #15744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.3
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15744 +/- ##
============================================
- Coverage 60.76% 60.76% -0.01%
+ Complexity 11727 11720 -7
============================================
Files 1938 1938
Lines 88637 88637
Branches 13380 13380
============================================
- Hits 53864 53857 -7
- Misses 29245 29247 +2
- Partials 5528 5533 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
It might be better that adding an unit test to prove this PR. |
|
Thanks for the review! I believe there might be a small misunderstanding...this PR doesn’t introduce new behavior that needs its own unit test. It fixes order-dependent flakiness in the existing test Before this change, the fail=0; total=0
for s in 7138 7351 7892 11170 15857 15869 17721 20421 21699 22631 24059 29905; do
echo "SEED=$s"
total=$((total+1))
SEED=$s ./mvnw -pl dubbo-common -DfailIfNoTests=true -DforkCount=1 -DreuseForks=false \
-Dsurefire.runOrder=random \
-Djunit.jupiter.testclass.order.default=org.junit.jupiter.api.ClassOrderer\$Random \
-Djunit.jupiter.testmethod.order.default=org.junit.jupiter.api.MethodOrderer\$Random \
-Djunit.jupiter.testclass.order.random.seed="$s" \
-Djunit.jupiter.testmethod.order.random.seed="$s" \
-Dtest=org.apache.dubbo.common.extension.ExtensionLoaderTest test \
>/dev/null || { echo "** FAILED on SEED=$s **"; fail=$((fail+1)); }
done
echo
echo "===== SUMMARY ====="
echo "Total seeds: $total"
echo "Failed: $fail"
echo "Passed: $((total - fail))"
echo "==================="These seeds reliably reproduce the flakiness on the pre-fix code. I attempted to simplify this to a single command (e.g., running the polluter → victim pair deterministically), but Surefire didn’t consistently preserve the inter-test interference needed to trigger the bug. The above loop was therefore the most reliable way to demonstrate the issue. After this fix, none of these seeds fail. The test is now stable under any random execution order. |
|
i mean you could write an junit test by calling java |
|
I will look into this and add a comment when I made changes |
What is the purpose of the change?
This PR fixes order-dependent flakiness (OD-Vic) in
ExtensionLoaderTest.testInjectExtensionThis test fails under randomized test execution orders due to multiple DemoImpl instances being registered in the shared ScopeBeanFactory of ApplicationModel.
Root Cause
testInjectExtensionand related tests (testGetOrDefaultExtension) both interacted with the same sharedScopeBeanFactory.Because bean registration was not idempotent, repeated or parallel test execution sometimes resulted in duplicate
DemoImplentries, leading to inconsistent injection results or assertion failures.What's Changed
These changes eliminate shared-state interference and make the test deterministic under any order or random seed.
Verification
You can try running the loop below on pre-fix and post-fix code:
The seeds were found by using a script I developed locally. Sometimes not all seeds fail on execution, but some will always fail on pre-fix code. However, on post-fix code none of the seeds fail.
Checklist